linux: fix HKDF TLS key derivation back to OpenSSL 3.0.8#1055
linux: fix HKDF TLS key derivation back to OpenSSL 3.0.8#1055igaw merged 1 commit intolinux-nvme:masterfrom
Conversation
cleech
force-pushed
the
hkdf-old-openssl
branch
from
ff4af0c to
bd7a3e0
Compare
Me neither. I think the current code is nicer to read. Maybe we could add a fallback implementation which is only selected for older version of OpenSSL. So when <= 3.3 get retired we can just rip out the the fallback implementation. |
Too much duplication, we'll get something compatible with 3.0.8 into a more palatable state. |
cleech
force-pushed
the
hkdf-old-openssl
branch
from
bd7a3e0 to
25c9797
Compare
|
Ok, how about a slightly different take. I think this is cleaner, and I ended up removing the hkdf_info_printf() function in favor of snprintf() calls because it just wasn't helping anymore. I removed all the changes to the compat functions, because the entire point of them is to not change. But that does mean that the compat tests will fail in CI. Given that the old code is not only out-of-spec, but inconsistent across big/little endian systems and across openssl versions, I'm still not in favor of trying to maintain it. |
hreinecke
left a comment
hreinecke
left a comment
There was a problem hiding this comment.
This really looks better now.
| *(uint16_t *)pos = htons(key_len & 0xFFFF); | ||
| pos += sizeof(uint16_t); | ||
|
|
||
| ret = snprintf(pos, 257, "%ctls13 HostNQN", 13); |
There was a problem hiding this comment.
'13' as argument? why? Please use 'strlen("tls13 HostNQN")' or something to make it clear that this is the length of the string.
| *(uint16_t *)pos = htons(key_len & 0xFFFF); | ||
| pos += sizeof(uint16_t); | ||
|
|
||
| ret = snprintf(pos, 257, "%ctls13 nvme-tls-psk", 18); |
There was a problem hiding this comment.
Same here, use 'strlen("tls13 nvme-tls-psk")' instead of hard-coded '18'.
Well, do we want to break existing users? And I am sure there are users out there. Would it possible to issue a warning that a identifier has the wrong format? |
I would agree with @cleech , we should keep the original implementation as-is and not try to modify it. (Kinda the point of a compat implementation). And we do know that it generates errors, so that's actually not unexpected. And if the compat implementation generates different results for different openssl implementations then we need to store the various test vectors, not modifying the algorithm. |
OpenSSL prior to 3.3.1 had an issue with EVP_PKEY_CTX_add1_hkdf_info() where it acted like a 'set1' function instead of an 'add1' as documented. Work around that by building the entire info vector outside of the OpenSSL API and only calling this function once. This removes the hkdf_info_printf helper, which ended up being more of a hinderence with this change, in favor of building the info vector with calls like snprintf(p, 257, "%c%s", strlen(s), s) where HkdfLabel.label and HkdfLabel.context have a maxiumum length of 256 each. Signed-off-by: Chris Leech <[email protected]>
cleech
force-pushed
the
hkdf-old-openssl
branch
from
25c9797 to
5c2cfd4
Compare
| if (hkdf_info_printf(ctx, "tls13 HostNQN") <= 0) { | ||
| pos += ret; | ||
|
|
||
| ret = snprintf(pos, HKDF_INFO_CONTEXT_MAX + 1, "%c%s", |
There was a problem hiding this comment.
The size argument for the second snprintf needs to take into account that the previous snprintf already used up a bit of the buffer, right?
There was a problem hiding this comment.
There are limits on each field and the entire buffer is allocated to allow for the maximum of both. So I think it's Ok.
There was a problem hiding this comment.
okay, but I let you fix the static code analyzer reports :)
|
Thanks! |
3 participants
Attempted fix for Issue #1053
I'm not crazy about rewriting things this way, but it passes the new tests when building with OpenSSL 3.0.8 and still works on my SPDK connection interoperability test.
OpenSSL prior to 3.3.1 had an issue with EVP_PKEY_CTX_add1_hkdf_info() where it acted like a 'set1' function instead of an 'add1' as documented. Work around that by building the entire info vector outside of the OpenSSL API and only calling this function once.